Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Change Sockets.recvfrom to return full InetAddr instead of just host #32729

Closed

Conversation

jonnystorm
Copy link
Contributor

When awaiting PDUs from as yet unknown UDP clients, it is necessary to know the port of the client's socket in order to reply to said client. Returning not only the source address but, also, the source port of received PDUs ensures we can reply to clients without relaying this information in application data.

@vtjnash
Copy link
Member

vtjnash commented Jul 30, 2019

I’m half inclined to make this as a breaking change (instead of adding 2), since the current recvfrom is fairly useless. Thoughts?

Anyways, 👍 for addressing this issue!

@jonnystorm
Copy link
Contributor Author

I'm perfectly content to wait for the next major revision if it leads to a more sensible recvfrom. If the consensus is for that direction, I'll happily adjust my PR.

Thanks, @vtjnash!

@jonnystorm jonnystorm force-pushed the add-new-recvfrom-with-port branch from ffc622e to f7303bf Compare July 30, 2019 14:06
@jonnystorm
Copy link
Contributor Author

I've now made this pull request a breaking change to the Sockets API. Please let me know if I should revert. Thanks!

@StefanKarpinski StefanKarpinski added io Involving the I/O subsystem: libuv, read, write, etc. minor change Marginal behavior change acceptable for a minor release triage This should be discussed on a triage call labels Jul 31, 2019
@JeffBezanson
Copy link
Member

Triage is OK with this. To do:

  • Add a compatibility note to the doc string.
  • Add an item to NEWS
  • Update tests

@JeffBezanson JeffBezanson added needs news A NEWS entry is required for this change and removed triage This should be discussed on a triage call labels Aug 1, 2019
@mbauman mbauman changed the title RFC: Add Sockets.recvfrom2 to return both host and port RFC: Change Sockets.recvfrom to return full InetAddr instead of just host Aug 1, 2019
@mbauman mbauman added the needs compat annotation Add !!! compat "Julia x.y" to the docstring label Aug 1, 2019
@jonnystorm
Copy link
Contributor Author

Oops... I'll get those updated this evening.

@jonnystorm
Copy link
Contributor Author

@JeffBezanson One question: the guidelines indicate that news updates should be in a separate commit but then suggest news updates in a regular PR might cause problems; should I update news in this PR as its own commit? Thanks!

@JeffBezanson
Copy link
Member

I think it only suggests a separate commit as a way for the NEWS item to reference its own PR number; the commits can be squashed after that. The other part only refers to NEWS-update.jl, which is for updating the links at the bottom of NEWS.

@jonnystorm jonnystorm force-pushed the add-new-recvfrom-with-port branch from f7303bf to 81709fd Compare August 1, 2019 23:19
@jonnystorm
Copy link
Contributor Author

Done. Let me know if I need to change or add anything else.

@JeffBezanson
Copy link
Member

Thanks. I edited the text a bit and merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
io Involving the I/O subsystem: libuv, read, write, etc. minor change Marginal behavior change acceptable for a minor release needs compat annotation Add !!! compat "Julia x.y" to the docstring needs news A NEWS entry is required for this change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants